feat: support disabling monitoring for RootSync and RepoSync#2233
feat: support disabling monitoring for RootSync and RepoSync#2233cowsking wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to disable monitoring in Config Sync by adding a monitoring field to the RepoSync and RootSync CRD specs, and conditionally omitting the otel-agent container and its volumes when disabled. However, a critical issue was identified in the metrics registration logic across pkg/metrics/register.go, pkg/kmetrics/register.go, and pkg/resourcegroup/controllers/metrics/register.go. Returning a nil exporter when DISABLE_MONITORING is active will cause runtime panics (nil pointer dereferences) in callers that invoke defer exporter.Shutdown(ctx) or use the exporter without a nil check. To resolve this, all callers must be updated to handle a nil exporter safely, or a safe no-op exporter should be returned.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
d603b59 to
e458faf
Compare
8651415 to
1df8436
Compare
|
/retest |
|
/test kpt-config-sync-presubmit-e2e-multi-repo-test-group3 |
|
/test kpt-config-sync-presubmit-e2e-multi-repo-test-group1 |
| } | ||
|
|
||
| // MonitoringSpec defines the monitoring configuration. | ||
| type MonitoringSpec struct { |
There was a problem hiding this comment.
The API descriptions for monitoring and enabled are currently a bit tautological (monitoring defines the monitoring configuration). Following Kubernetes API documentation conventions, it would be clearer to describe the exact behavior, affected sidecars, and defaults
| otelCredentialProvider := &auth.CachingCredentialProvider{ | ||
| Scopes: traceapi.DefaultAuthScopes(), | ||
| } | ||
| if os.Getenv("DISABLE_MONITORING") != "true" { |
There was a problem hiding this comment.
Notice that the CRD API uses positive naming (spec.monitoring.enabled: false) while the internal environment variable uses negative opt-out naming (DISABLE_MONITORING=true). While spec.monitoring.enabled follows K8s API conventions and DISABLE_MONITORING=true provides backward-compatible defaults when unset, the difference might confuse contributors. Could we add a short code comment in pkg/reconcilermanager/controllers/util.go explaining this naming rationale?
| "github.com/GoogleContainerTools/config-sync/pkg/reconcilermanager" | ||
| ) | ||
|
|
||
| func deploymentMissingVolume(volumeName string) testpredicates.Predicate { |
There was a problem hiding this comment.
Suggest moving the predicates to e2e/nomostest/testpredicates/predicates.go
|
|
||
| // RegisterOTelExporter creates the OTLP metrics exporter. | ||
| func RegisterOTelExporter(ctx context.Context, containerName string) (*otlpmetricgrpc.Exporter, error) { | ||
| if os.Getenv("DISABLE_MONITORING") == "true" { |
There was a problem hiding this comment.
Meant to be "!="? I wonder why the test was not catching this
There was a problem hiding this comment.
In cmd/reconciler-manager/main.go, we use if os.Getenv("DISABLE_MONITORING") != "true" because we are conditionally executing and wrapping a block of code (and persisting general initialization flow afterwards).
Here in pkg/kmetrics/register.go (and pkg/metrics/register.go), we use == "true" as an early return to guard clause. If monitoring is explicitly disabled, we initialize the safe Noop variables and return early. The rest of the function operates under the assumption that monitoring is active.
Returning nil here is by design, no exporter exists when monitoring is disabled. All four call sites in this PR already nil-check OTel Collector Exporter before Shutdown, so this can't panic.
|
|
||
| // RegisterOTelExporter creates the OTLP metrics exporter. | ||
| func RegisterOTelExporter(ctx context.Context, containerName string) (*otlpmetricgrpc.Exporter, error) { | ||
| if os.Getenv("DISABLE_MONITORING") == "true" { |
| func RegisterOTelExporter(ctx context.Context, containerName string) (*otlpmetricgrpc.Exporter, error) { | ||
| if os.Getenv("DISABLE_MONITORING") == "true" { | ||
| klog.V(5).Infof("METRIC DEBUG: Monitoring is disabled via DISABLE_MONITORING env var") | ||
| err := InitializeOTelMetrics() |
There was a problem hiding this comment.
Why init if monitoring is disabled?
There was a problem hiding this comment.
The recording functions in record.go call these global instruments unconditionally and skipping init would leave them nil and panic on the first Record() call. Since we skip SetMeterProvider here, the instruments fall back to the global no-op provider, so recordings are safe no-ops and nothing is exported. Added a comment in the code to clarify.
1df8436 to
f4d1df5
Compare
This commit introduces the ability to opt-out of monitoring sidecars (specifically otel-agent) for RootSync and RepoSync objects via the spec.monitoring.enabled field in the declarative API. When monitoring is disabled, the config-sync controllers will omit the otel-agent container from the reconciler pods and set the environment variable DISABLE_MONITORING=true in the remaining vital containers to silence telemetry exporter logic. All Unit and E2E tests for this logic have been implemented accordingly. TAG=agy
f4d1df5 to
2ba5c87
Compare
|
@cowsking: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
2 participants
This commit introduces the ability to opt-out of monitoring sidecars (specifically otel-agent) for RootSync and RepoSync objects via the
spec.monitoring.enabled fieldin the declarative API.When monitoring is disabled, the config-sync controllers will omit the otel-agent container from the reconciler pods and set the environment variable
DISABLE_MONITORING=truein the remaining vital containers to silence telemetry exporter logic. All Unit and E2E tests for this logic have been implemented accordingly.